Ensure LMS passes correct group_id when querying content #5088

dan-f · 2014-09-04T22:03:06Z

TNL-23

dan-f · 2014-09-08T13:02:31Z

@jimabramson @gwprice @andy-armstrong @cahrens please review

dan-f · 2014-09-08T13:05:24Z

lms/djangoapps/django_comment_client/tests/group_id.py

+    Provides test cases to verify that views pass the correct `group_id` to
+    the comments service when querying discussion topics.
+    """
+    cs_call_num = -1  # by default, inspect the last call to the comments service


I'd like some feedback on the possible refactoring of these mixins, in particular the assertion logic. The problem is that we need to check calls to mock_request to assert that it was/wasn't called with "group_id". However, the order of the calls depends on the particular view.

@gwprice

One way to do this that would be much less brittle would be to check the request URL instead of looking for a specific index in the list of calls. You could define cs_endpoint instead of cs_call_num and then do something like mock_request.call_args.filter(lambda args: args[1].endswith(self.cs_endpoint))[0]

Aha! I considered that, but was thinking of matching the beginning of the URL rather than the end, which posed problems when tests would query different discussion topics. Your solution should solve that :)

gwprice · 2014-09-08T14:43:28Z

lms/lib/comment_client/thread.py

@@ -119,6 +119,7 @@ def _retrieve(self, *args, **kwargs):
            'mark_as_read': kwargs.get('mark_as_read', True),
            'resp_skip': kwargs.get('response_skip'),
            'resp_limit': kwargs.get('response_limit'),
+            'group_id': kwargs.get('group_id')


The group_id parameter should not be passed to the CS when retrieving a single thread

If we don't pass 'group_id', then any user will be able to see any thread. With my changes, 'group_id' in this place will be the student's actual group_id.

The group_id parameter is meaningless in this comments service endpoint. The constraint needs to be enforced at a higher level (i.e. once the thread is retrieved, check its group_id against the user's group_id and return 404 if there's no match).

dan-f · 2014-09-08T17:30:35Z

@gwprice thanks for the feedback. I've addressed your comments in my last commit.

gwprice · 2014-09-08T17:34:05Z

lms/lib/comment_client/thread.py

@@ -118,7 +118,7 @@ def _retrieve(self, *args, **kwargs):
            'user_id': kwargs.get('user_id'),
            'mark_as_read': kwargs.get('mark_as_read', True),
            'resp_skip': kwargs.get('response_skip'),
-            'resp_limit': kwargs.get('response_limit'),
+            'resp_limit': kwargs.get('response_limit')


Remove this change

gwprice · 2014-09-08T17:41:23Z

lms/djangoapps/django_comment_client/tests/utils.py

-                "cohorted topic": {"id": "cohorted_topic"},
-                "non-cohorted topic": {"id": "non_cohorted_topic"},
+                "cohorted_topic": {"id": "cohorted_topic"},
+                "non_cohorted_topic": {"id": "non_cohorted_topic"},


Why are you changing these?

Oops... I must have thought that those were topic IDs rather than topic names.

gwprice · 2014-09-08T18:53:25Z

I dislike the amount of repeated code in the mixin classes. We basically have the same set of test cases enumerated three times (without topic, with cohorted topic, and with non-cohorted topic). I see a couple of potential solutions. One would be to have something like the following:

class GroupIdTestMixin(object):
    <define assertions and test cases using call_view>

class CohortedTopicGroupIdTestMixin(GroupIdTestMixin):
    def call_view(...):
        self.call_view_with_topic("cohorted_topic", ...)

class NonCohortedTopicGroupIdTestMixin(GroupIdTestMixin):
    def call_view(...):
        self.call_view_with_topic("non_cohorted_topic", ...)

The biggest problem I have with this approach is that the two mixin subclasses could not be used in the same TestCase.

Another approach could be to use ddt to get test cases for the two different topics and then have non-topic-dependent test cases just ignore the topic parameter. This would keep the amount of code down but result in redundant tests.

dan-f · 2014-09-08T21:12:13Z

I think I'm going to go the ddt route. It doesn't make sense to me to have two test classes per semantic test case (i.e. one test class for cohorted topic and one for a non-cohorted topic).

Edit see comment below

dan-f · 2014-09-09T00:54:49Z

@gwprice I think there's a limit to how much we can factor this. While the group_id scenarios are semantically the same across the three cases in terms of setup, the assertions are different between the "cohorted" and "non-cohorted" topic cases. I tried using ddt to define all test cases in one mixin, but it required conditional assertions and the "non-cohorted" topic assertions failed for the "without topic" test case. I think the best that can be done is to have the following:

class GroupIdAssertionMixin(object):
    <define assertions>

class CohortedTopicGroupIdTestMixin(GroupIdTestMixin):
    # ^ not sure what to call this one
    <define test cases for "without topic" / "cohorted topic" cases using call_view()>

class NonCohortedTopicGroupIdTestMixin(GroupIdTestMixin):
    <define test cases for non-cohorted topic cases using call_view()>

Note that CohortedTopicGroupIdTestMixin and NonCohortedTopicGroupIdTestMixin can be used in the same TestCase.

gwprice · 2014-09-09T03:03:57Z

Right, I should have realized that the assertions limit the ability to refactor. I like your suggestion.

dan-f · 2014-09-09T14:10:15Z

@gwprice I have refactored as we discussed. Thanks again for the thorough review :) Please let me know whether or not you have any remaining concerns.

gwprice · 2014-09-09T17:48:50Z

lms/djangoapps/django_comment_client/forum/tests.py

+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class GetThreadsGroupIdTestCase(


get_threads is really supposed to be a module-private function, so we shouldn't write separate tests for it

Should I really remove them? It feels like a bad idea not to test such an integral helper function.

It should be adequately tested by the view tests.

dan-f · 2014-09-10T00:31:08Z

@gwprice please see my latest changes

gwprice · 2014-09-10T03:44:49Z

lms/djangoapps/django_comment_client/forum/tests.py

@@ -333,6 +329,198 @@ def test_html(self, mock_request):
        self.assertRegexpMatches(html, r'&quot;group_name&quot;: &quot;student_cohort&quot;')


+@patch('lms.lib.comment_client.utils.requests.request')
+class SingleThreadAccessTestCase(CohortedContentTestCase, GroupIdAssertionMixin):


This does not need to inherit from GroupIdAssertionMixin

dan-f · 2014-09-10T13:22:09Z

@gwprice

gwprice · 2014-09-10T16:17:21Z

👍

jimabramson · 2014-09-10T19:41:36Z

👍

Ensure LMS passes correct group_id when querying content

dan-f changed the title ~~[WIP] Test how LMS queries comments_service with group_id~~ [WIP] Ensure LMS passes correct group_id when querying content Sep 4, 2014

dan-f force-pushed the dan-f/test-lms-passes-group-id branch from d66db24 to 3a5dab6 Compare September 8, 2014 13:01

dan-f reviewed Sep 8, 2014
View reviewed changes

dan-f changed the title ~~[WIP] Ensure LMS passes correct group_id when querying content~~ Ensure LMS passes correct group_id when querying content Sep 8, 2014

gwprice reviewed Sep 8, 2014
View reviewed changes

dan-f force-pushed the dan-f/test-lms-passes-group-id branch from a299e29 to bec7564 Compare September 8, 2014 17:39

gwprice reviewed Sep 8, 2014
View reviewed changes

gwprice reviewed Sep 9, 2014
View reviewed changes

gwprice reviewed Sep 10, 2014
View reviewed changes

dan-f force-pushed the dan-f/test-lms-passes-group-id branch from c81e9e5 to c597be8 Compare September 10, 2014 17:13

Ensure LMS passes correct group_id when querying content

4d9517b

dan-f force-pushed the dan-f/test-lms-passes-group-id branch from b653bd8 to 4d9517b Compare September 10, 2014 20:43

dan-f added a commit that referenced this pull request Sep 10, 2014

Merge pull request #5088 from edx/dan-f/test-lms-passes-group-id

95a06d9

Ensure LMS passes correct group_id when querying content

dan-f merged commit 95a06d9 into master Sep 10, 2014

dan-f deleted the dan-f/test-lms-passes-group-id branch September 10, 2014 21:18

snyk-bot mentioned this pull request Oct 4, 2021

[Snyk] Upgrade uglify-js from 2.7.0 to 3.14.2 baby636/edx-platform#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure LMS passes correct group_id when querying content #5088

Ensure LMS passes correct group_id when querying content #5088

dan-f commented Sep 4, 2014

dan-f commented Sep 8, 2014

dan-f Sep 8, 2014

gwprice Sep 8, 2014

dan-f Sep 8, 2014

gwprice Sep 8, 2014

dan-f Sep 8, 2014

gwprice Sep 8, 2014

dan-f commented Sep 8, 2014

gwprice Sep 8, 2014

dan-f Sep 8, 2014

gwprice Sep 8, 2014

dan-f Sep 8, 2014

gwprice commented Sep 8, 2014

dan-f commented Sep 8, 2014

dan-f commented Sep 9, 2014

gwprice commented Sep 9, 2014

dan-f commented Sep 9, 2014

gwprice Sep 9, 2014

dan-f Sep 9, 2014

gwprice Sep 9, 2014

dan-f commented Sep 10, 2014

gwprice Sep 10, 2014

dan-f commented Sep 10, 2014

gwprice commented Sep 10, 2014

jimabramson commented Sep 10, 2014



		@patch('lms.lib.comment_client.utils.requests.request')
		class GetThreadsGroupIdTestCase(

Ensure LMS passes correct group_id when querying content #5088

Ensure LMS passes correct group_id when querying content #5088

Conversation

dan-f commented Sep 4, 2014

dan-f commented Sep 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Sep 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwprice commented Sep 8, 2014

dan-f commented Sep 8, 2014

dan-f commented Sep 9, 2014

gwprice commented Sep 9, 2014

dan-f commented Sep 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Sep 10, 2014

Choose a reason for hiding this comment

dan-f commented Sep 10, 2014

gwprice commented Sep 10, 2014

jimabramson commented Sep 10, 2014